Operators to extend interactivity with python objects #12
Conversation
There was a problem hiding this comment.
Left several comments, my overall feeling is that it might be easier if we turn each of these into its own PR, since there are design decisions with all of them which might require extended discussion.
For the LDS package, I think it actually might be more practical to just implement what you need directly as C# nodes in the package, to avoid breaking changes later.
| [WorkflowElementCategory(ElementCategory.Transform)] | ||
| public class Args | ||
| { | ||
| public IObservable<Pythonnet.PyTuple> Process(IObservable<object> source) |
There was a problem hiding this comment.
It would both be more idiomatic and performant here to have strongly-typed overloads for compatible types, or implement the operator using a full-blown ExpressionBuilder.
This way we wouldn't have to rely on dynamic type checking and reflection to know how to construct the PyTuple object, since we would know the exact types at compile time. We could then automatically generate code that builds the tuple for the exact type signature and skip all the expensive reflection calls altogether.
There was a problem hiding this comment.
Are these changes required? They seem unrelated to the new operators, but I wonder whether you may have had trouble compiling in Linux perhaps?
|
|
||
| public IObservable<PyObject> Process(IObservable<PyObject> source) | ||
| { | ||
| if (string.IsNullOrEmpty(Attribute)) |
There was a problem hiding this comment.
If you are testing here the Attribute property it might be best to cache its value locally, otherwise if the property is externalized and changing dynamically, someone could set its value to null while the sequence is running and accidentally bypass this test.
| { | ||
| if (string.IsNullOrEmpty(Attribute)) | ||
| { | ||
| throw new Exception("Attribute cannot be null or empty."); |
There was a problem hiding this comment.
The framework design guidelines recommend not throwing the Exception base type. Since properties are not function arguments, for bonsai operators we have converged on using InvalidOperationException as the convention for operator state validation errors.
| { | ||
| if (!obj.HasAttr(Attribute)) | ||
| { | ||
| throw new Exception($"PyObject does not have attribute {Attribute}."); |
There was a problem hiding this comment.
Do we need to check and throw here? I would expect that simply calling GetAttr would throw some kind of python exception if the attribute does not exist.
There was a problem hiding this comment.
Same comment as GetItem and InvokeMethod re. making the PyObject be the property.
| { | ||
| if (Value == null) | ||
| { | ||
| throw new Exception("Value cannot be null."); |
There was a problem hiding this comment.
Same comment re. exception typing and caching.
| public class SetItem | ||
| { | ||
| [Description("The index to set.")] | ||
| public int Index { get; set; } = 0; |
There was a problem hiding this comment.
Same comment re. variable types of indexers. The current implementation seems to only support int, but we know string also works, and possibly many other types. Flipping the logic around and making PyObject the property would keep things more flexible.
| { | ||
| if (!Pythonnet.PySequence.IsSequenceType(obj)) | ||
| { | ||
| throw new Exception($"PyObject is not a type of sequence."); |
There was a problem hiding this comment.
Same comment regarding preventive testing, I would hope in all these cases that pythonnet would provide some kind of sane failure exception for incompatible types, especially given the dynamic nature of python, so we shouldn't have to pretest any objects.
| } | ||
| if (Value == null) | ||
| { | ||
| throw new Exception("Value cannot be null."); |
There was a problem hiding this comment.
Interesting, so you can't you set a value to None? Maybe pythonnet doesn't accept null directly, but wondering whether here we should in that case convert null to the None singleton?
This PR is intended to add initial support for extended interactivity and manipulation of python objects outside of python scripting. Below are a few highlighted key features to demonstrate these new nodes.
Basic python primitives
Some nodes provide the ability to instantiate python primitives, such as
PyInt,PyFloat, andPyString.Import
It is possible to use the
Importfunction to import packages directly into a module. For example, this would be the equivalent of usingimport [python-package]in a script.Invoke
Invokecan be used to instantiate a callable object. It can work directly on python objects to instantiate them or it can be used to invoke an attribute of the object. It supports passingArgsto the invocation.InvokeMethod
InvokeMethodcan be used to call a method of an object. Supports passingArgs.Args
Argswill take a single python object or a tuple/list/array of python objects and convert it to a type of PyTuple which can be passed asArgsto a function.GetAttr
The
GetAttrmethod can be used to access an attribute of a python object.Example with numpy
Take the example of creating a numpy array. In python, you would do something like this.
To achieve the equivalent python script, you can do the following. You can import the np library object to
Invokeand set theCallableproperty toarray, passing aPyIntasArgs. Here is how that might look:Then, if you wanted to use the
transpose, you can pass the resulting numpy array object toInvokeMethodand specify theMethodproperty to betranspose. Alternatively, instead of calling thetransposefunction of the numpy array, you could callGetAttrwithAttributeset toT, which is the equivalent of performing a transpose usingarr.Tin python.